Conversation
a02ec8e to
3244d7a
Compare
| return exact_match if exact_match | ||
|
|
||
| if route_matches.present? | ||
| exact_route_match || best_prefix_match |
There was a problem hiding this comment.
Could we end up up favouring a live root edition when there's a draft based on whatever other ordering we do here? Is that correct?
There was a problem hiding this comment.
question: could exact_route_match now return two matches, one from each content store?
There was a problem hiding this comment.
This is a tricky bit of code to grasp. I'm sort of hopeful @brucebolt might have an idea as the person who implemented this class (though I did review that code 🙈), but otherwise I think this will needs more investigation
|
|
||
| context "when the source Edition is live" do | ||
| context "when requested with with_drafts=true" do | ||
| it "includes a draft 'en' link if there isn't a draft locale-matching one" do |
There was a problem hiding this comment.
thought: these precedence tests might be superseded by repurposing the thorough set of tests in 20b1c9a, so that they check against agreed outcomes instead of checking for agreement with the non-graphql implementation of link expansion.
There was a problem hiding this comment.
Do you mean that we would write in some specific expectations in the precedence tests? Are the expectations regular enough to do that at the required scale (there are many thousands of tests)?
I do still feel the precedence tests should only be semi-permanent though - they're quite beastly and unconventional, and if we keep their scope mostly down to Content Store comparison, it'll be easier to say goodbye down the line. These more conventional tests already document intended behaviour, though it's quite possible (probable?) that we're missing a few edge cases that the precedence tests cover implicitly
There was a problem hiding this comment.
I didn't put this very well. My train of thought was:
- Is the non-root-non-default-locale edition in this test necessary?
- Not if it's only testing inclusion, not precedence.
- If it's only testing inclusion, and we remove the non-root-non-default-locale edition from the test, then it'll duplicate a test that's in Test link expansion inclusion rules with truth tables (direct links) #3886
- But if it's intended to test precedence, not just inclusion, then maybe other precedence cases should be tested too, such as to test whether a link-set link to a draft edition in the default locale has precedence over an edition link to a live edition in the root locale.
Related to #3908 (comment)
I don't think this discussion should block this PR being merged though.
21d7f42 to
e34f034
Compare
84a66bf to
6995f80
Compare
6995f80 to
5162df3
Compare
| context "when with_drafts=false" do | ||
| it "doesn't return links to drafts" do | ||
| target_edition_1 = create(:draft_edition, title: "edition 1, draft") | ||
| target_document = create(:document) | ||
| target_edition_2 = create(:live_edition, title: "edition 2, live", document: target_document) | ||
| create(:draft_edition, title: "edition 3, draft", document: target_document) | ||
|
|
||
| source_edition = create(:live_edition, | ||
| links_kind => [ | ||
| { link_type: "test_link", target_content_id: target_edition_1.content_id }, | ||
| { link_type: "test_link", target_content_id: target_document.content_id }, | ||
| ]) | ||
|
|
||
| expected_titles = [target_edition_2.title] | ||
| expect(source_edition).to have_links("test_link").with_titles(expected_titles) | ||
| end | ||
| end |
There was a problem hiding this comment.
suggestion: the precedence aspect of this test might be clearer if separated from the other one, e.g. if the precedence was described as it "when drafts=false, it returns links to live editions despite drafts being available".
There was a problem hiding this comment.
Or maybe the precedence aspect of this test is already covered by defaults to including a (live) 'en' link if the locale-matching one is draft below.
There was a problem hiding this comment.
Yeah this test isn't really about precedence - target edition 1 isn't competing, for instance. I do wonder if it's a bit of a messy test (see also: #3803 (comment)) but perhaps it's useful having one competing (the unassigned one) and one non-competing target edition
There was a problem hiding this comment.
Since we've both found this confusing, perhaps it's worth splitting the non-confusing, non-precedence part of the test off, so that at least that part of it is clearer.
I agree there's a need for a test case that uses a competing live edition to tempt graphql to return a draft. Especially if the draft appears to be superior in some aspect such as locale or unpublished status. But I'll save that argument for my defence of the thousands of precedence tests.
|
|
||
| context "when the source Edition is live" do | ||
| context "when requested with with_drafts=true" do | ||
| it "includes a draft 'en' link if there isn't a draft locale-matching one" do |
There was a problem hiding this comment.
thought: these precedence tests might be superseded by repurposing the thorough set of tests in 20b1c9a, so that they check against agreed outcomes instead of checking for agreement with the non-graphql implementation of link expansion.
| expect(target_edition).to have_reverse_links("test_link").with_titles(expected_titles) | ||
| end | ||
|
|
||
| it "returns reverse links to live editions when drafts aren't available" do |
There was a problem hiding this comment.
Is this meant to take me to a comment? It seems to take me to a commit
There was a problem hiding this comment.
There was a problem hiding this comment.
Oh about whether it had been fixed up?
nacnudus
left a comment
There was a problem hiding this comment.
It's patronising of me to praise this, but I will anyway.
There might be a corner case in the edition finder service, when there are editions in both content stores that don't have an exactly-matching base path, but do have an exactly-matching route. I don't understand why there would be such things, so I've left it as a question.
| return exact_match if exact_match | ||
|
|
||
| if route_matches.present? | ||
| exact_route_match || best_prefix_match |
There was a problem hiding this comment.
question: could exact_route_match now return two matches, one from each content store?
| DEFAULT_TTL = ENV.fetch("DEFAULT_TTL", 5.minutes).to_i.seconds | ||
| MINIMUM_TTL = [DEFAULT_TTL, 5.seconds].min | ||
|
|
||
| def draft_content |
There was a problem hiding this comment.
I see the argument in the commit message for having the duplication here, but I do wonder if there's potential for deviation from each other in the future?
Given there's only (as far as I can see) two lines difference between live and draft, would DRY-ing up these methods (with some conditional logic for the two lines that are different) be a better approach?
5162df3 to
e9af2e9
Compare
When introducing support for drafts, using the default draft edition factory will become an issue
This was missed in 7ece3e9
So that it doesn't conflict with the have_links matcher in the other dataloader's spec
We're working towards supporting draft content via GraphQL. It seems reasonable to have this scenario covered for the #live_content controller action, whether we choose to extend this endpoint or create a separate one.
The `live_edition` and `draft_edition` (alias `edition`) spec factories were both defaulting to a user_facing_version of 1. Since a document's live and draft editions are required to have different user_facing_versions, this new default of 2 enables a spec example to create both states of editions for a single document without introducing the noise of explicitly setting this attribute for one or both of them. We can rethink this if it begins to make other tests more awkward in future. Note: that the unpublished_edition factory needed updating here too. This was to prevent its default user_facing_version from changing since it inherits from the factory we've changed.
e9af2e9 to
62e4865
Compare
| GraphQL::Dataloader.with_dataloading do |dataloader| | ||
| request = dataloader.with( | ||
| Sources::LinkedToEditionsSource, | ||
| content_store: with_drafts ? "draft" : "live", |
There was a problem hiding this comment.
We should move these changes to the two commits that change the dataloaders
| AND editions.document_type NOT IN (:non_renderable_formats) | ||
| AND ( | ||
| editions.state != 'unpublished' | ||
| editions.state IN (:state_unless_withdrawn) |
There was a problem hiding this comment.
I'm finding this variable name a bit hard to read and ambiguous. I think we tend to aim to name variables to describe what they contain, while this one is focused around the condition that created it.
I'm wondering if something like visible_states, allowed_states or permitted_states would make it more descriptive.
There was a problem hiding this comment.
Yeah, I'm not so keen on this name either. It doesn't represent all allowed states though - unpublished is also allowed and these are the other allowed states. The condition that created it here is whether we're including drafts rather than anything about the unpublishing type (withdrawn etc). Perhaps something wordier like :permitted_not_unpublished_states 🤔
There was a problem hiding this comment.
It's a tricky one. The other allowed/permitted states are handled in the OR block.
It's basically live OR live + draft when drafts are included, so -not_unpublished would capture it.
This replaces the content_store parameter. The behaviour of this dataloader is the same between the old default of `content_store: "live"` and the new default of `with_drafts: false`. `content_store: "draft"` won't behave like `with_drafts: true`, though, since live editions can now be included in a draft edition's links (if a more suitable draft version doesn't exist). In the near future the flag will be set in the query. For now we omit the argument when called in the link methods defined via `Types::BaseObject`, thereby defaulting to false. The inclusion tests are also updated to use the new parameter to check drafts. Co-authored-by: Ynda Jas <yndajas@gmail.com>
This replaces the content_store parameter. The behaviour of this dataloader is the same between the old default of `content_store: "live"` and the new default of `with_drafts: false`. `content_store: "draft"` won't behave like `with_drafts: true`, though, since live editions can now be included in a draft edition's links (if a more suitable draft version doesn't exist). In the near future the flag will be set in the query. For now we omit the argument when called in the link methods defined via `Types::BaseObject` and `Types::EditionQuery`, thereby defaulting to false. The inclusion tests are also updated to use the new parameter to check drafts. Co-authored-by: Ynda Jas <yndajas@gmail.com> Check drafts in reverse link expansion inclusion tests
Draft editions can legitimately have null first_published_at and/or public_updated_at. They follow different rules, but I've found Edition::Timestamps and UpdateExistingDraftEdition to be useful reading on the topic
The `content_store` variable isn't currently being used and we're dropping support for it. Its replacement (`with_drafts`) is coming soon
As with the dataloader sources, this content_store setting was never used and it defaulted to "live". A query with `content_store: "live"` always returned a live edition or no edition at all. That's the same behaviour as `with_drafts: false`. A query with `content_store: "draft"` always returned a draft edition or no edition. `with_drafts: true` doesn't behave like that because it can return a draft edition or a live edition or no edition, with that order of precedence. In Content Store land, there's a draft content item (i.e. in Draft Content Store) for every published content item (i.e. in Live Content Store). This `with_drafts` fallback behaviour is intended to mimick the user-visible results of that. Unlike the dataloader sources, which have to consider a linked-to edition's state, QueryType looks up the root edition by base_path, which will only have 0 or 1 exact matches for each of `content_store: "live"` and `content_store: "draft"`.
Until recently, the dataloaders would only link editions together that had matching `content_store`s. Since introducing `with_drafts` to the dataloaders, we've been omitting the argument in the link methods defined in `Types::BaseObject` and thereby defaulting to `false`, only including live linked editions. Now, using the query's `with_drafts` setting in the dataloaders, when `with_drafts: true`, any draft edition can link to a live edition and vice versa, at any level of nesting in the generated response.
We're working towards supporting draft content via GraphQL using the `with_drafts` flag. Any GraphQL queries we generate in future should have support for this variable to allow drafts to be served.
We're working towards supporting draft content via GraphQL using the `with_drafts` flag. Including this variable in a query enables clients to set the variable and it ultimately ends up being used by the QueryType and dataloaders.
All existing uses of this class specify content_store: "live". Note: the line that changed in the controller action resulted in an entry in the Brakeman ignore file needing updating. I ran `brakeman -I` to remove the old version and introduce the new version.
This flag replaces the content_store parameter. With `content_store: "live"` this service always returned a live edition or no edition at all. That's the same behaviour as `with_drafts: false`. With `content_store: "draft"`, this service always returned a draft edition or no edition. `with_drafts: true` doesn't behave like that exactly, it can return a draft edition or a live edition or no edition, with that order of precedence. I don't think there's any need to compare a published edition with an unpublished edition at any point in this service. For base_paths that match edition records exactly, there can't be more than 1 record with that same base_path and `content_store: "live"`. So I believe we're expecting 0 or 1 draft editions and 0 or 1 live editions. This service then does its own sorting once it gets into the routes and redirects, so I *think* any more sorting by state or content_store precedence is moot. I'm hoping that the matching rules should take precedence over them, anyway, but it might be possible that we need to modify this code to group pairs of sibling draft and live editions? Co-authored-by: Ynda Jas <yndajas@gmail.com>
In a subsequent commit we'll introduce a draft content action which will duplicate most of the behaviour of the live content action, so this makes it easier to extend to other actions by wrapping most of the behaviour in shared examples
Which behaves a lot like the /graphql/content live content endpoint, only with `with_drafts: true`. I've left all the duplication here because it's tricky to see where to begin with that. If we settle on having 2 separate controller actions like this, then we can spend the time to make it easier and less error prone to work with. One consequence of the duplication is that it includes another file access Brakeman warning to ignore (as the code that we duplicated did). I've introduced a spec example here that demonstrates how this endpoint's use of with_drafts=true enables a live edition to link to a draft edition and vice versa. I've put it in its own separate integration spec because it seems less focussed on endpoint behaviour than the controller specs since it covers QueryType and dataloader behaviour. Co-authored-by: Ynda Jas <yndajas@gmail.com>
These tests provide us with reassurance that we're in line with Content Store other than in a few known scenarios. The new examples added by considering drafts all pass, suggesting there are no draft-specific divergences from Content Store (unless any of our existing ignore rules are masking novel diffs)
These tests provide us with reassurance that we're in line with Content Store other than in a few known scenarios. The new examples added by considering drafts all pass, suggesting there are no draft-specific divergences from Content Store (unless any of our existing ignore rules are masking novel diffs)
For the GraphQL link expansion integration tests, in CI we have one job for each of the four specs to speed things up. However, when introducing drafts the number of examples in each spec increases substantially, leading to each of the specs taking longer than the job for the main test suite, and about two minutes longer for the different content ID specs We can double down on the parallelisation by introducing the parallel_rspec gem, which by default runs four examples at a time (if the machine has enough cores - GitHub Actions does by default). This doesn't neatly divide the run time by four but it does bring the GraphQL link expansion integration test run times in line with or below the main test suite The main test suite fails when running examples in parallel, so we can't currently use this to speed up the overall CI run time In order to run the examples in parallel, the gem creates three additional databases. When doing this preparation it tries to drop the initial database, which fails if the initial database is using the default name of 'postgres' we think because the postgres server relies on this default-named database. Therefore for these jobs we use a non-default name for the initial database (the specific name of which doesn't matter beyond not being 'postgres')
62e4865 to
4577adf
Compare
| .order( | ||
| Arel.sql( | ||
| <<~SQL, | ||
| CASE editions.content_store | ||
| WHEN 'draft' THEN 0 | ||
| WHEN 'live' THEN 1 | ||
| ELSE 2 | ||
| END | ||
| SQL | ||
| ), | ||
| ) |
There was a problem hiding this comment.
The commit message ("with_drafts: true doesn't behave like that because it can return a draft edition or a live edition or no edition") suggests that we don't want to return any other results than draft or live, so we don't want the ELSE 2.
Cleaner, rails-native suggestion:
| .order( | |
| Arel.sql( | |
| <<~SQL, | |
| CASE editions.content_store | |
| WHEN 'draft' THEN 0 | |
| WHEN 'live' THEN 1 | |
| ELSE 2 | |
| END | |
| SQL | |
| ), | |
| ) | |
| .in_order_of(:content_store, %w[draft live]) |
There was a problem hiding this comment.
Yeah, I wondered something similar with state in Mike's earlier PR: #3803 (comment). In practice the .where(base_path:, content_store: content_stores) will address this but agree there's no need for the else. Didn't know about in_order_of - that's much nicer!
There was a problem hiding this comment.
We should probably also do this in app/services/edition_finder_service.rb
Per the linked comment above, the ELSE on state in the dataloader SQL queries can probably go too, though it wasn't introduced in this PR

Introduces support for draft content in GraphQL. This involves changes to the GraphQL queries, the dataloaders and their SQL queries, the GraphQL controller, a few GraphQL types, and the edition finder service. Various refactors are included along the way. More detail in the individual commit messages
The draft content endpoint will not yet be exposed via GDS API Adapters. We need to think about authentication/authorisation and probably do some diffing (though the link expansion precedence tests introduced in #3876 and #3883 give us some confidence that we're broadly in line with Content Store for link expansion except in a few known scenarios). We'll also need to add support in frontend apps once supported by GDS API Adapters
Closes #3803